feat(engine)!: add TakeFromBucket instruction + wallet multiple transfers - #1614
Conversation
WalkthroughRefactors stealth transfer requests to use a per-transfer Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Handler
participant SDK
participant TxBuilder
participant Processor
participant Runtime
rect rgb(220, 255, 220)
note over Client,SDK: New stealth transfer flow (per-transfer collection)
Client->>Handler: POST StealthTransferRequest(transfers: [StealthTransfer...])
Handler->>SDK: StealthTransferParams(outputs: Vec<TransferOutput>)
SDK->>SDK: validate outputs, determine per-output accounts
SDK->>TxBuilder: add per-output instructions (create account? deposit)
TxBuilder->>Processor: submit transaction
Processor->>Runtime: execute instructions (including TakeFromBucket if present)
Runtime->>Processor: put_on_workspace / get / put results
end
sequenceDiagram
autonumber
participant User
participant TransactionBuilder
participant Processor
participant Workspace
User->>TransactionBuilder: take_from_bucket(label, amount, output_label)
TransactionBuilder->>TransactionBuilder: resolve workspace ids, append Instruction::TakeFromBucket
TransactionBuilder->>Processor: submit transaction
rect rgb(255, 240, 200)
note over Processor,Workspace: TakeFromBucket execution
Processor->>Workspace: Get(input_bucket)
Workspace-->>Processor: return Bucket
Processor->>Processor: perform BucketAction::Take(amount)
Processor->>Workspace: put_on_workspace(output_bucket, result_bucket)
Workspace-->>Processor: ack
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30–40 minutes Changes are cross-cutting (bindings, SDK, runtime, proto, builder, tests) and introduce a new instruction plus validation/parameter refactors; review requires verifying type/serialization consistency and runtime behavior but follows consistent patterns. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
crates/template_test_tooling/src/read_only_state_store.rs (1)
54-63: Consider defensive error handling for type conversions.The implementation correctly collects all resources, but relies on
unwrap()at line 58 assuming that aSubstateId::Resourcealways corresponds to an actualResourcesubstate. While this is likely guaranteed by system invariants, consider using?with proper error propagation orok_or_else()for more defensive programming.Optional refactor example:
pub fn get_all_resources(&self) -> Result<HashMap<ResourceAddress, Resource>, StateStoreError> { let mut resources = HashMap::new(); self.with_substates(|id, substate| { if let SubstateId::Resource(resource_address) = id { - let resource = substate.substate_value().as_resource().unwrap(); + if let Some(resource) = substate.substate_value().as_resource() { resources.insert(*resource_address, resource.clone()); + } } })?; Ok(resources) }crates/wallet/sdk/src/apis/stealth_transfer/params.rs (1)
93-99: Use checked arithmetic for Amount to prevent overflow.total_output_amount() and the sums in total_output_amount()/total_revealed_output_amount() use plain addition. Prefer checked/saturating math (or an explicit overflow guard in validate) to avoid undefined behavior if Amount can overflow.
- If Amount provides checked_add/saturating_add, use it and map overflow to StealthTransferApiError::AmountOverflow.
- Otherwise, add a validate-phase guard that rejects outputs where revealed_amount + blinded_amount would overflow. Based on learnings.
Also applies to: 116-118
crates/wallet/sdk/src/apis/stealth_transfer/types.rs (3)
14-21: Add Debug and Clone derives to StealthTransferOutput.This struct serves as the main output type for stealth transfers. Adding
DebugandClonederives will improve debuggability and usability, especially when this type needs to be passed around or inspected during development.Apply this diff:
+#[derive(Debug, Clone)] pub struct StealthTransferOutput { pub transaction: UnsignedTransaction, pub lock_id: WalletLockId, pub fee_inputs: InputsToSpend, pub transfer_inputs: InputsToSpend, pub additional_signer: Option<WalletPublicKey>, pub main_signer: WalletPublicKey, }
23-32: Consider adding Clone derive if UnblindedStealthInputWitness supports it.Adding
Clonewould provide flexibility when these inputs need to be referenced in multiple contexts during transaction construction.If the wrapped
UnblindedStealthInputWitnessimplementsClone, apply this diff:-#[derive(Debug)] +#[derive(Debug, Clone)] pub struct UnblindedInputToSpend { pub witness: UnblindedStealthInputWitness, }
61-65: Add Debug and Clone derives to AccountDetails.This struct should have basic derives for better usability and debuggability.
Apply this diff:
+#[derive(Debug, Clone)] pub struct AccountDetails { pub address: ComponentAddress, pub vaults: Vec<VaultId>, pub exists: bool, }crates/engine/tests/test.rs (1)
99-108: Unused parameter in closure signature.The closure signature was updated to include a first parameter that is unused (indicated by
_). Consider documenting what this parameter represents, as it may confuse developers updating existing code.If the first parameter is consistently unused in most cases, consider whether the API could provide both a single-parameter and two-parameter overload, or document the purpose of the first parameter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
applications/tari_walletd/src/handlers/accounts.rs(2 hunks)applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts(1 hunks)bindings/package.json(1 hunks)bindings/src/types/Instruction.ts(1 hunks)bindings/src/types/tari-indexer-client/IndexerGetIdentityResponse.ts(1 hunks)bindings/src/types/wallet-daemon-client/StealthTransfer.ts(1 hunks)bindings/src/types/wallet-daemon-client/StealthTransferRequest.ts(1 hunks)bindings/src/wallet-daemon-client.ts(1 hunks)clients/javascript/wallet_daemon_client/package.json(1 hunks)clients/wallet_daemon_client/src/types.rs(1 hunks)crates/engine/src/runtime/impl.rs(3 hunks)crates/engine/src/runtime/mod.rs(1 hunks)crates/engine/src/runtime/working_state.rs(1 hunks)crates/engine/src/transaction/processor.rs(2 hunks)crates/engine/tests/account.rs(1 hunks)crates/engine/tests/test.rs(1 hunks)crates/p2p/proto/transaction.proto(2 hunks)crates/p2p/src/conversions/transaction.rs(2 hunks)crates/template_lib/src/component/instance.rs(1 hunks)crates/template_lib/src/resource/builder/fungible.rs(1 hunks)crates/template_test_tooling/src/read_only_state_store.rs(2 hunks)crates/template_test_tooling/templates/faucet/src/lib.rs(1 hunks)crates/transaction/src/builder/mod.rs(1 hunks)crates/transaction/src/v1/instruction.rs(2 hunks)crates/transaction/src/v1/transaction.rs(2 hunks)crates/wallet/sdk/src/apis/stealth_outputs.rs(3 hunks)crates/wallet/sdk/src/apis/stealth_transfer/api.rs(13 hunks)crates/wallet/sdk/src/apis/stealth_transfer/error.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_transfer/mod.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_transfer/params.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_transfer/types.rs(1 hunks)integration_tests/src/wallet_daemon_client.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (23)
crates/engine/tests/account.rs (4)
crates/transaction/src/builder/mod.rs (2)
take_from_bucket(259-272)new(53-59)crates/template_test_tooling/src/read_only_state_store.rs (1)
new(21-23)crates/transaction/src/transaction.rs (2)
new(51-53)builder(47-49)crates/template_test_tooling/src/template_test.rs (1)
owner_proof(404-406)
crates/engine/src/runtime/mod.rs (2)
crates/engine/src/runtime/impl.rs (1)
put_on_workspace(2697-2705)bindings/src/types/IndexedValue.ts (1)
IndexedValue(4-4)
crates/transaction/src/builder/mod.rs (2)
crates/engine/tests/account.rs (1)
take_from_bucket(282-331)bindings/src/types/Instruction.ts (1)
Instruction(15-42)
crates/template_test_tooling/templates/faucet/src/lib.rs (3)
crates/template_lib/src/resource/builder/fungible.rs (1)
new(64-75)crates/template_lib/src/resource/builder/mod.rs (1)
public_fungible(54-56)crates/template_lib/src/component/instance.rs (2)
new(24-31)new(98-100)
crates/p2p/src/conversions/transaction.rs (3)
crates/engine/tests/account.rs (1)
take_from_bucket(282-331)crates/transaction/src/builder/mod.rs (1)
take_from_bucket(259-272)bindings/src/types/Instruction.ts (1)
Instruction(15-42)
integration_tests/src/wallet_daemon_client.rs (2)
bindings/src/types/wallet-daemon-client/StealthTransfer.ts (1)
StealthTransfer(6-11)crates/template_lib/src/component/instance.rs (1)
address(116-118)
bindings/src/types/Instruction.ts (2)
bindings/src/types/WorkspaceOffsetId.ts (1)
WorkspaceOffsetId(3-3)bindings/src/types/Amount.ts (1)
Amount(12-12)
bindings/src/types/wallet-daemon-client/StealthTransferRequest.ts (4)
bindings/src/types/wallet-daemon-client/ComponentAddressOrName.ts (1)
ComponentAddressOrName(4-4)bindings/src/types/ConfidentialTransferInputSelection.ts (1)
ConfidentialTransferInputSelection(3-7)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)bindings/src/types/wallet-daemon-client/StealthTransfer.ts (1)
StealthTransfer(6-11)
applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts (1)
crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
params(675-687)
crates/wallet/sdk/src/apis/stealth_transfer/mod.rs (2)
crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
params(675-687)crates/wallet/sdk/src/apis/mod.rs (1)
stealth_transfer(16-16)
bindings/src/types/wallet-daemon-client/StealthTransfer.ts (2)
bindings/src/types/OotleAddress.ts (1)
OotleAddress(3-3)bindings/src/types/Amount.ts (1)
Amount(12-12)
applications/tari_walletd/src/handlers/accounts.rs (1)
crates/wallet/sdk/src/apis/stealth_transfer/api.rs (1)
transfer(287-587)
crates/template_lib/src/resource/builder/fungible.rs (4)
crates/template_lib/src/component/instance.rs (3)
with_address_allocation(34-36)address(116-118)with_address_allocation_opt(39-45)crates/template_lib/src/resource/builder/confidential.rs (1)
with_address_allocation(79-82)crates/template_lib/src/resource/builder/non_fungible.rs (1)
with_address_allocation(77-80)crates/template_lib/src/resource/builder/stealth.rs (1)
with_address_allocation(80-83)
crates/template_test_tooling/src/read_only_state_store.rs (4)
bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)bindings/src/types/Resource.ts (1)
Resource(9-22)bindings/src/types/SubstateId.ts (1)
SubstateId(6-6)bindings/src/types/Substate.ts (1)
Substate(4-4)
crates/transaction/src/v1/transaction.rs (1)
bindings/src/types/Instruction.ts (1)
Instruction(15-42)
clients/wallet_daemon_client/src/types.rs (2)
bindings/src/types/wallet-daemon-client/StealthTransfer.ts (1)
StealthTransfer(6-11)bindings/src/types/OotleAddress.ts (1)
OotleAddress(3-3)
crates/wallet/sdk/src/apis/stealth_transfer/error.rs (1)
crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
is_not_found_error(780-782)
crates/transaction/src/v1/instruction.rs (2)
bindings/src/types/WorkspaceOffsetId.ts (1)
WorkspaceOffsetId(3-3)bindings/src/types/Amount.ts (1)
Amount(12-12)
crates/wallet/sdk/src/apis/stealth_transfer/params.rs (6)
bindings/src/types/OotleAddress.ts (1)
OotleAddress(3-3)bindings/src/types/Network.ts (1)
Network(6-6)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/ConfidentialTransferInputSelection.ts (1)
ConfidentialTransferInputSelection(3-7)crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
outputs(691-691)
crates/wallet/sdk/src/apis/stealth_transfer/types.rs (6)
crates/wallet/crypto/src/unblinded_statement.rs (1)
memo(68-70)bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)bindings/src/types/VaultId.ts (1)
VaultId(6-6)bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/UnsignedTransaction.ts (1)
UnsignedTransaction(4-4)crates/p2p/src/conversions/transaction.rs (2)
value(830-834)value(891-895)
crates/template_lib/src/component/instance.rs (1)
crates/template_lib/src/resource/builder/fungible.rs (2)
with_address_allocation(120-122)with_address_allocation_opt(125-128)
crates/engine/src/transaction/processor.rs (4)
bindings/src/types/Instruction.ts (1)
Instruction(15-42)crates/template_lib/src/models/bucket.rs (1)
amount(175-183)bindings/src/types/IndexedValue.ts (1)
IndexedValue(4-4)bindings/src/types/InstructionResult.ts (1)
InstructionResult(5-5)
crates/wallet/sdk/src/apis/stealth_transfer/api.rs (1)
crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
params(675-687)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: check nightly
- GitHub Check: test
- GitHub Check: machete
- GitHub Check: clippy
- GitHub Check: check stable
- GitHub Check: fmt
🔇 Additional comments (40)
bindings/src/types/tari-indexer-client/IndexerGetIdentityResponse.ts (1)
7-7: Array notation is consistent across ts-rs generated files—no action needed.Verification confirms that
Array<string>notation is systematically used across all ts-rs generated type files in the bindings directory (6 instances found across multiple directories), with no instances ofstring[]notation. This indicates the change is the expected output of the ts-rs code generator, not a manual stylistic edit. The file is correctly generated and consistent with the codebase.crates/template_test_tooling/src/read_only_state_store.rs (1)
90-94: LGTM: Improved API design.The signature change from
FnMut(Substate)toFnMut(&SubstateId, &Substate)is an improvement that provides more context to callers while avoiding unnecessary cloning during iteration. The implementation correctly forwards both parameters to the closure.clients/wallet_daemon_client/src/types.rs (2)
1070-1078: LGTM! Well-structured type definition.The
StealthTransferstruct clearly encapsulates per-transfer data with appropriate field types. Theskip_serializing_ifattribute onoutput_memois a good optimization for optional fields.
1064-1064: No changes needed—empty transfers are already validated.The code properly validates that the transfers vector is non-empty. The
StealthTransferParams::validate()function checksif self.outputs.is_empty()and returns an error with the message "At least one output must be specified" if no outputs are provided. This validation is called in the handler before processing the request.crates/template_test_tooling/templates/faucet/src/lib.rs (2)
38-40: LGTM! Clean delegation pattern.The refactor maintains backward compatibility while enabling the new optional address allocation functionality.
42-56: LGTM! Well-structured API extension.The new
mint_with_optsfunction correctly extends the faucet API to support optional address allocation without breaking existing consumers.crates/template_lib/src/resource/builder/fungible.rs (1)
120-122: LGTM! Appropriate builder pattern update.Changing to consume
selfby value and delegating to the optional variant is a clean refactor that maintains the fluent builder API.crates/template_lib/src/component/instance.rs (2)
34-36: LGTM! Clean builder pattern refactor.The change to consume
selfand delegate to the optional variant maintains the fluent builder API while centralizing the allocation logic.
39-45: LGTM! Excellent defensive implementation.The assertion at Line 41 prevents accidental overwriting of an existing allocation, providing clear feedback if the builder is misconfigured. The
Nonecase safely no-ops, making this pattern robust.crates/wallet/sdk/src/apis/stealth_transfer/error.rs (1)
18-44: LGTM: coherent error surface.Enum is well-structured and composes upstream APIs cleanly; IsNotFoundError impl matches store semantics.
applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts (1)
118-130: <!-- [scratchpad_start]
[task verification_1/1 check_request_contract_3/3]
Status: Verification complete. All findings gathered.
[observations]
- StealthTransferRequest struct (line 1060-1068):
- owner_account: ComponentAddressOrName ✓
- transfers: Vec ✓
- Matches review expectation
- StealthTransfer struct (1072-1078):
- destination_address: OotleAddress ✓
- blinded_output_amount: Amount ✗ (NOT number - this is the issue!)
- revealed_output_amount: Amount ✗ (NOT number - this is the issue!)
- output_memo: Option ✓
- Amount type: exports as
string | numberin TypeScript per previous search- Current implementation (lines 153-167):
- Uses "owner_account" field name correctly ✓
- Passes params.amount as number directly to Amount fields
- No type conversion to Amount - values just cast implicitly
- TransferParams.amount typed as number (line 120), should be Amount
[analysis]
CORRECT ISSUES IDENTIFIED:
- TransferParams.amount is number but should be Amount to prevent precision loss
- The implementation in useAccounts.ts passes number values to Amount-typed fields
- Amount supports string | number in TS, but proper typing prevents runtime errors
- The owner_account field name is ALREADY CORRECT (not a bug in current code)
VERIFIED FACTS:
- Server expects owner_account (confirmed ✓)
- Server expects transfers array with these exact fields (confirmed ✓)
- Amount type should be used for blinded_output_amount and revealed_output_amount (confirmed ✓)
- Current code already uses owner_account correctly at line 154 (already fixed or was never wrong)
[done]
Ready to generate rewritten review comment. The review comment is mostly correct but needs refinement:
- The field name concern (owner_account vs account) is ALREADY addressed in the implementation
- The Amount typing concern is the main actionable issue remaining
</scratchpad_end] -->Tighten amount typing from
numbertoAmount; server contract verified.The code correctly uses
owner_accountand the transfers array structure matches the server contract. However,TransferParams.amountshould use theAmounttype instead ofnumberto prevent precision loss for large values:- amount: number; + amount: Amount; // from @tari-project/typescript-bindingsThe
blinded_output_amountandrevealed_output_amountfields in the transfers array (lines 160–161) are correctly mapped toAmounttypes server-side; usingAmountat the parameter level ensures type safety and consistency.crates/wallet/sdk/src/apis/stealth_transfer/types.rs (1)
34-39: LGTM!The struct is well-designed with appropriate derives and a lifetime parameter for the memo reference.
crates/transaction/src/v1/transaction.rs (2)
191-191: LGTM!The weight of 1 for
TakeFromBucketis consistent with other simple workspace operations likeAssertBucketContainsandClaimValidatorFees.
199-202: Baseline weight added for stealth transfers.The addition of a fixed baseline weight of 100 for stealth transfer statements reflects the cryptographic overhead of these operations. This is a reasonable approach given the computational cost of validating stealth proofs.
bindings/src/wallet-daemon-client.ts (1)
35-35: LGTM!The export of
StealthTransferis correctly placed and consistent with the existing export structure.crates/transaction/src/v1/instruction.rs (2)
75-79: LGTM!The
TakeFromBucketinstruction variant is well-structured with appropriate fields for specifying the source bucket, amount to take, and destination bucket.
190-200: LGTM!The Display implementation for
TakeFromBucketproperly formats all three fields, maintaining consistency with other instruction Display implementations.crates/engine/src/runtime/working_state.rs (1)
399-406: Improved dangling bucket validation.The validation now correctly counts only non-zero buckets as dangling, rather than all buckets. This is more accurate since empty buckets don't represent leaked resources and shouldn't cause transaction failures.
crates/engine/src/runtime/mod.rs (1)
192-192: New RuntimeInterface method added.The
put_on_workspacemethod extends the runtime API to support placing values directly on the workspace by ID. This is a breaking change to theRuntimeInterfacetrait, but is necessary to support the newTakeFromBucketinstruction flow.crates/engine/tests/account.rs (1)
281-331: LGTM! Comprehensive test for TakeFromBucket.The test effectively covers:
- Creating a faucet and taking free coins
- Splitting a bucket into two parts using
TakeFromBucket- Testing the edge case where the source bucket is fully drained (line 302 comment)
- Verifying final account balances
The test validates that empty buckets don't cause dangling bucket errors, which aligns with the working_state.rs validation changes.
bindings/src/types/Instruction.ts (1)
24-33: TakeFromBucket variant wired correctly.Nice to see the bindings keeping pace with the engine changes so clients can emit the new instruction without hand-written glue. Nothing further from my side.
bindings/src/types/wallet-daemon-client/StealthTransferRequest.ts (1)
7-13: Aggregated transfers shape looks good.Thanks for moving the request over to the
transfersarray; this matches the backend refactor and keeps the types tidy.bindings/src/types/wallet-daemon-client/StealthTransfer.ts (1)
1-11: Handshake between Rust and TS stays aligned.Appreciate the dedicated
StealthTransfertype; makes the client payload much clearer.crates/engine/src/transaction/processor.rs (1)
319-336: Runtime hook for TakeFromBucket looks solid.The workspace fetch + bucket take + reinsert flow mirrors the existing engine patterns, so I don’t see any gaps here.
crates/engine/src/runtime/impl.rs (2)
1950-1955: Nice touch onstate.new_bucket_id().Switching to the helper keeps bucket allocation consistent with the other call sites.
2697-2705: Handyput_on_workspacehelper.Centralising the insert logic with validation makes the new instruction cleaner.
crates/transaction/src/builder/mod.rs (1)
259-272: Builder API addition fits right in.This will make composing multi-output flows far less awkward; thanks for the ergonomic win.
applications/tari_walletd/src/handlers/accounts.rs (2)
26-33: Wallet-side refactor matches the new SDK params.Mapping the request transfers straight into
TransferOutputkeeps the handler clean and future-proof.
971-986: End-to-end payload assembly still lines up.The new
outputscollection is passed straight through to the SDK, so the daemon remains in lockstep with the client types.crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
44-44: LGTM! Type rename and borrowing optimization.The refactor from
OutputToCreatetoStealthOutputToCreatemakes the type more specific and self-documenting. The borrowing change on line 680 (&output.owner_addressinstead of moving) is a good optimization that avoids unnecessary clones while maintaining safety through the lifetime parameter'a.Also applies to: 667-667, 680-680
crates/p2p/src/conversions/transaction.rs (2)
236-244: LGTM! Proper error handling and validation.The proto-to-internal conversion correctly:
- Validates required
input_bucketfield with descriptive error message- Handles optional
amountwith sensible default- Protects against
output_bucketoverflow with proper contextFollows the established pattern from other instructions like
AssertBucketContains.
388-400: LGTM! Consistent serialization implementation.The internal-to-proto conversion properly wraps all fields and follows the same structure as other instructions in this file.
crates/p2p/proto/transaction.proto (1)
35-40: Breaking change properly documented.The field number renumbering for existing instruction variants (lines 35-39) and the new
TakeFromBucketmessage definition (lines 65-69) constitute a breaking protocol change. This is acknowledged in the PR description with "Requires deletion of the data directory."The new message structure correctly aligns with the conversion implementation in
crates/p2p/src/conversions/transaction.rs.Also applies to: 65-69
integration_tests/src/wallet_daemon_client.rs (1)
126-131: LGTM! Test correctly updated for multi-transfer API.The refactoring from flat transfer fields to a
transferscollection properly supports the new capability of sending to multiple recipients in a single transaction. The test correctly creates a single-element vector to match the new API signature.crates/wallet/sdk/src/apis/stealth_transfer/api.rs (6)
319-328: LGTM! Pre-determining account creation needs is efficient.This loop gathers which destination accounts need to be created before transaction construction. This approach allows the transaction builder to make informed decisions about whether to create accounts or deposit to existing ones.
The use of
HashSetforaccounts_to_createprovides O(1) lookup during transaction building.
589-676: Well-structured account existence check with proper network fallback.The function correctly handles multiple scenarios:
- ✅ Early exit for stealth-only transfers (no revealed funds)
- ✅ Local account check with on-chain confirmation validation
- ✅ Network lookup as fallback
- ✅ Proper vault substate inclusion for existing accounts
The logic properly adds both the account and vault (if exists) to
substate_inputs, ensuring the transaction has required inputs for deposit operations.
507-516: Output ordering is critical for change detection logic.The chaining order (explicit outputs → change output → filter) is important because line 522 relies on
.last()to access the change output. This works correctly because:
change_outputis chained after all explicit outputs- The filter preserves order
- If change amount is positive, it will be the last element
However, this implicit ordering dependency is subtle and could be fragile.
Consider adding an assertion or comment documenting this ordering requirement:
// NOTE: change_output must be chained last because we access it via .last() below (line 522) outputs: params .outputs .iter() .map(Into::into) .chain(change_output) .filter(|o| o.amount.is_positive()),
742-764: Transaction builder correctly handles conditional account creation.The logic properly:
- ✅ Iterates over outputs with index for unique bucket naming
- ✅ Skips outputs with no revealed amount
- ✅ Uses
take_from_bucketto split the output bucket by exact revealed amount- ✅ Creates account with bucket for new accounts (using
accounts_to_createset)- ✅ Deposits to existing accounts otherwise
The use of
sub_bucket_namewith index ensures unique workspace keys.
359-364: LGTM! Consistent with API refactoring.The change to
StealthOutputToCreatewithowner_address.clone()is necessary because the type now uses borrowing semantics (as seen in stealth_outputs.rs). The clone here is unavoidable since we need to hold the value across iterations.Also applies to: 491-495
692-699: Good addition of network parameter for transaction building.The
networkparameter (line 692) is properly used on line 704 to set the transaction network via.for_network(network.as_byte()). Theaccounts_to_createparameter enables the conditional account creation logic.
1e66877 to
c369a45
Compare
c369a45 to
935bb22
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/template_lib/src/resource/builder/fungible.rs (1)
120-128: Consider adding defensive check for duplicate allocation.The
with_address_allocation_optmethod directly assigns the address without checking if one already exists, as previously noted. The component builder includes an assertion to prevent accidental overwrites atcrates/template_lib/src/component/instance.rslines 38-44.Apply this diff to add the defensive check:
pub fn with_address_allocation_opt(mut self, address: Option<ResourceAddressAllocation>) -> Self { - self.address_allocation = address; + if let Some(address) = address { + assert!(self.address_allocation.is_none(), "Address allocation already set"); + self.address_allocation = Some(address); + } self }bindings/package.json (1)
3-3: [DUPLICATE] Version bump must be 2.0.0 (major), not 1.19.0 (minor)—breaking API change still unresolved.This is a continuation of the critical issue flagged in the prior review. Backward incompatible API changes increment the major version. The StealthTransferRequest structure change (removal of flat fields:
destination_address,blinded_output_amount,revealed_output_amount,output_memoand replacement withtransfers: Array<StealthTransfer>) is a breaking change that requires a major version bump per semver specification.Update the version to
"2.0.0"and ensure release notes document the breaking changes for consumers.
🧹 Nitpick comments (5)
crates/template_lib/src/resource/builder/confidential.rs (1)
79-87: Consider adding defensive check for duplicate allocation.The
with_address_allocation_optmethod directly assigns the address without checking if one already exists. The component builder (atcrates/template_lib/src/component/instance.rslines 38-44) includes an assertion to prevent accidental overwrites. Adding the same check here would catch builder misuse earlier.Apply this diff to add the defensive check:
pub fn with_address_allocation_opt(mut self, address: Option<ResourceAddressAllocation>) -> Self { - self.address_allocation = address; + if let Some(address) = address { + assert!(self.address_allocation.is_none(), "Address allocation already set"); + self.address_allocation = Some(address); + } self }crates/template_lib/src/resource/builder/stealth.rs (1)
80-88: Consider adding defensive check for duplicate allocation.The
with_address_allocation_optmethod directly assigns the address without checking if one already exists. The component builder (atcrates/template_lib/src/component/instance.rslines 38-44) includes an assertion to prevent accidental overwrites. Adding the same check here would improve consistency and catch builder misuse earlier.Apply this diff to add the defensive check:
pub fn with_address_allocation_opt(mut self, address: Option<ResourceAddressAllocation>) -> Self { - self.address_allocation = address; + if let Some(address) = address { + assert!(self.address_allocation.is_none(), "Address allocation already set"); + self.address_allocation = Some(address); + } self }crates/template_lib/src/resource/builder/non_fungible.rs (1)
77-85: Consider adding defensive check for duplicate allocation.The
with_address_allocation_optmethod directly assigns the address without checking if one already exists. The component builder (atcrates/template_lib/src/component/instance.rslines 38-44) includes an assertion to prevent accidental overwrites. Adding the same check here would improve consistency and catch builder misuse earlier.Apply this diff to add the defensive check:
pub fn with_address_allocation_opt(mut self, address: Option<ResourceAddressAllocation>) -> Self { - self.address_allocation = address; + if let Some(address) = address { + assert!(self.address_allocation.is_none(), "Address allocation already set"); + self.address_allocation = Some(address); + } self }crates/transaction/src/v1/transaction.rs (1)
199-202: Clarify the rationale for the 100 baseline in stealth statement weight.A baseline of 100 has been added to the stealth statement weight calculation. While this likely accounts for fixed overhead (e.g., balance proof verification), the rationale should be documented.
Consider adding a constant and documentation:
+// Fixed cost for stealth transfer statement verification (balance proof, etc.) +const STEALTH_STATEMENT_BASE_WEIGHT: u64 = 100; + fn calc_stealth_statement_weight(statement: &StealthTransferStatement) -> u64 { // TODO: weight inputs and outputs accordingly - currently outputs cost 2x inputs - 100 + statement.inputs_statement.inputs.len() as u64 + (statement.outputs_statement.outputs.len() as u64 * 2) + STEALTH_STATEMENT_BASE_WEIGHT + + statement.inputs_statement.inputs.len() as u64 + + (statement.outputs_statement.outputs.len() as u64 * 2) }crates/transaction/src/builder/mod.rs (1)
259-272: Consider validating the amount parameter.The implementation correctly integrates with the workspace and instruction systems. However, there's no validation on the
amountparameter.Consider adding validation to catch errors early:
pub fn take_from_bucket<T: Into<BuilderWorkspaceKey>, A: Into<Amount>>( mut self, label: T, amount: A, output_label: T, ) -> Self { let key = self.get_workspace_offset_id_from_named_arg(label.into()); let output_key = self.workspace_ids.insert(output_label.into()); + let amount = amount.into(); + // Validate amount is positive (negative amounts should use different operations) + if amount.is_negative() { + panic!("take_from_bucket amount must be non-negative"); + } self.add_instruction(Instruction::TakeFromBucket { input_bucket: key, - amount: amount.into(), + amount, output_bucket: output_key, }) }Note: This validation may already exist at the runtime level, so this is optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
Cargo.toml(1 hunks)applications/tari_walletd/src/handlers/accounts.rs(2 hunks)applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts(1 hunks)bindings/package.json(1 hunks)bindings/src/types/Instruction.ts(1 hunks)bindings/src/types/tari-indexer-client/IndexerGetIdentityResponse.ts(1 hunks)bindings/src/types/wallet-daemon-client/StealthTransfer.ts(1 hunks)bindings/src/types/wallet-daemon-client/StealthTransferRequest.ts(1 hunks)bindings/src/wallet-daemon-client.ts(1 hunks)clients/javascript/wallet_daemon_client/package.json(1 hunks)clients/wallet_daemon_client/src/types.rs(1 hunks)crates/engine/src/runtime/impl.rs(3 hunks)crates/engine/src/runtime/mod.rs(1 hunks)crates/engine/src/runtime/working_state.rs(1 hunks)crates/engine/src/transaction/processor.rs(2 hunks)crates/engine/tests/account.rs(1 hunks)crates/engine/tests/test.rs(1 hunks)crates/p2p/proto/transaction.proto(2 hunks)crates/p2p/src/conversions/transaction.rs(2 hunks)crates/template_lib/src/component/instance.rs(1 hunks)crates/template_lib/src/resource/builder/confidential.rs(1 hunks)crates/template_lib/src/resource/builder/fungible.rs(1 hunks)crates/template_lib/src/resource/builder/non_fungible.rs(1 hunks)crates/template_lib/src/resource/builder/stealth.rs(1 hunks)crates/template_test_tooling/src/read_only_state_store.rs(2 hunks)crates/template_test_tooling/templates/faucet/src/lib.rs(1 hunks)crates/transaction/src/builder/mod.rs(1 hunks)crates/transaction/src/v1/instruction.rs(2 hunks)crates/transaction/src/v1/transaction.rs(2 hunks)crates/wallet/sdk/src/apis/stealth_outputs.rs(3 hunks)crates/wallet/sdk/src/apis/stealth_transfer/api.rs(13 hunks)crates/wallet/sdk/src/apis/stealth_transfer/error.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_transfer/mod.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_transfer/params.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_transfer/types.rs(1 hunks)integration_tests/src/wallet_daemon_client.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (19)
- crates/transaction/src/v1/instruction.rs
- crates/template_test_tooling/src/read_only_state_store.rs
- bindings/src/types/wallet-daemon-client/StealthTransfer.ts
- bindings/src/types/tari-indexer-client/IndexerGetIdentityResponse.ts
- clients/wallet_daemon_client/src/types.rs
- bindings/src/types/Instruction.ts
- crates/wallet/sdk/src/apis/stealth_transfer/types.rs
- applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts
- integration_tests/src/wallet_daemon_client.rs
- crates/p2p/src/conversions/transaction.rs
- crates/engine/src/runtime/working_state.rs
- crates/wallet/sdk/src/apis/stealth_outputs.rs
- crates/wallet/sdk/src/apis/stealth_transfer/error.rs
- crates/template_lib/src/component/instance.rs
- crates/wallet/sdk/src/apis/stealth_transfer/mod.rs
- clients/javascript/wallet_daemon_client/package.json
- crates/engine/tests/account.rs
- crates/template_test_tooling/templates/faucet/src/lib.rs
- crates/engine/src/runtime/impl.rs
🧰 Additional context used
🧬 Code graph analysis (12)
crates/template_lib/src/resource/builder/fungible.rs (4)
crates/template_lib/src/component/instance.rs (3)
with_address_allocation(34-36)address(116-118)with_address_allocation_opt(39-45)crates/template_lib/src/resource/builder/confidential.rs (2)
with_address_allocation(79-81)with_address_allocation_opt(84-87)crates/template_lib/src/resource/builder/non_fungible.rs (2)
with_address_allocation(77-79)with_address_allocation_opt(82-85)crates/template_lib/src/resource/builder/stealth.rs (2)
with_address_allocation(80-82)with_address_allocation_opt(85-88)
crates/engine/src/runtime/mod.rs (2)
crates/engine/src/runtime/impl.rs (1)
put_on_workspace(2697-2705)bindings/src/types/IndexedValue.ts (1)
IndexedValue(4-4)
crates/transaction/src/v1/transaction.rs (1)
bindings/src/types/Instruction.ts (1)
Instruction(15-42)
crates/template_lib/src/resource/builder/confidential.rs (4)
crates/template_lib/src/component/instance.rs (3)
with_address_allocation(34-36)address(116-118)with_address_allocation_opt(39-45)crates/template_lib/src/resource/builder/fungible.rs (2)
with_address_allocation(120-122)with_address_allocation_opt(125-128)crates/template_lib/src/resource/builder/non_fungible.rs (2)
with_address_allocation(77-79)with_address_allocation_opt(82-85)crates/template_lib/src/resource/builder/stealth.rs (2)
with_address_allocation(80-82)with_address_allocation_opt(85-88)
applications/tari_walletd/src/handlers/accounts.rs (1)
crates/wallet/sdk/src/apis/stealth_transfer/api.rs (1)
transfer(287-591)
crates/wallet/sdk/src/apis/stealth_transfer/params.rs (6)
bindings/src/types/OotleAddress.ts (1)
OotleAddress(3-3)bindings/src/types/Network.ts (1)
Network(6-6)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)bindings/src/types/Amount.ts (1)
Amount(12-12)integration_tests/src/wallet_daemon_client.rs (1)
confidential_transfer(867-910)bindings/src/types/ConfidentialTransferInputSelection.ts (1)
ConfidentialTransferInputSelection(3-7)
crates/template_lib/src/resource/builder/non_fungible.rs (4)
crates/template_lib/src/component/instance.rs (3)
with_address_allocation(34-36)address(116-118)with_address_allocation_opt(39-45)crates/template_lib/src/resource/builder/confidential.rs (2)
with_address_allocation(79-81)with_address_allocation_opt(84-87)crates/template_lib/src/resource/builder/fungible.rs (2)
with_address_allocation(120-122)with_address_allocation_opt(125-128)crates/template_lib/src/resource/builder/stealth.rs (2)
with_address_allocation(80-82)with_address_allocation_opt(85-88)
crates/engine/src/transaction/processor.rs (5)
bindings/src/types/Instruction.ts (1)
Instruction(15-42)crates/engine_types/src/resource_container.rs (1)
amount(134-141)crates/template_lib/src/models/bucket.rs (1)
amount(175-183)bindings/src/types/IndexedValue.ts (1)
IndexedValue(4-4)bindings/src/types/InstructionResult.ts (1)
InstructionResult(5-5)
crates/template_lib/src/resource/builder/stealth.rs (4)
crates/template_lib/src/component/instance.rs (3)
with_address_allocation(34-36)address(116-118)with_address_allocation_opt(39-45)crates/template_lib/src/resource/builder/confidential.rs (2)
with_address_allocation(79-81)with_address_allocation_opt(84-87)crates/template_lib/src/resource/builder/fungible.rs (2)
with_address_allocation(120-122)with_address_allocation_opt(125-128)crates/template_lib/src/resource/builder/non_fungible.rs (2)
with_address_allocation(77-79)with_address_allocation_opt(82-85)
bindings/src/types/wallet-daemon-client/StealthTransferRequest.ts (4)
bindings/src/types/wallet-daemon-client/ComponentAddressOrName.ts (1)
ComponentAddressOrName(4-4)bindings/src/types/ConfidentialTransferInputSelection.ts (1)
ConfidentialTransferInputSelection(3-7)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)bindings/src/types/wallet-daemon-client/StealthTransfer.ts (1)
StealthTransfer(6-11)
crates/transaction/src/builder/mod.rs (3)
crates/engine/tests/account.rs (1)
take_from_bucket(282-331)bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/Instruction.ts (1)
Instruction(15-42)
crates/wallet/sdk/src/apis/stealth_transfer/api.rs (1)
crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
params(675-687)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test
- GitHub Check: machete
- GitHub Check: check nightly
- GitHub Check: check stable
- GitHub Check: clippy
- GitHub Check: fmt
🔇 Additional comments (18)
crates/engine/tests/test.rs (2)
99-99: LGTM! Closure signature updated correctly.The closure signature has been updated to match the new
with_substatesAPI. The first parameter is correctly marked as unused with the underscore prefix.
99-110: Based on my verification:The removal of the
nft_indexesmodule is intentional and complete.The search found no references to
nft_indexes,nft_index,setup_nft_index, ortest_new_nft_indexanywhere in the codebase.Test coverage for NFT functionality is actively maintained through:
mod basic_nft(lines 506-891): Tests core NFT functionality including resource creation, minting, burning, and mutable data updatesmod emoji_id(lines 893-1025): Tests NFT creation with specific ID formats (emoji-based)mod tickets(lines 1026+): Tests NFT use case with state mutations and access control- Integration tests: Extensive NFT operations (mint, list, get, transfer) in
integration_tests/tests/cucumber.rsand wallet daemon handlersThe NFT indexing functionality itself continues to be supported in the runtime, storage, and wallet SDK layers. The removal only affects the dedicated test module, and alternative test coverage is comprehensive.
bindings/src/wallet-daemon-client.ts (1)
35-35: LGTM! StealthTransfer export added to public API.The export properly exposes the new
StealthTransfertype, enabling consumers to use it with the updatedStealthTransferRequestthat now accepts an array of transfers.crates/engine/src/runtime/mod.rs (1)
192-192: LGTM! Workspace write capability added to RuntimeInterface.The new
put_on_workspacemethod properly extends the runtime interface to support writingIndexedValueto a workspace. This is used by the newTakeFromBucketinstruction to place the output bucket onto the workspace.bindings/src/types/wallet-daemon-client/StealthTransferRequest.ts (1)
5-14: LGTM! API refactored to support multiple transfers.The refactoring successfully enables multiple stealth transfers in a single transaction by:
- Replacing single
destination_addresswithtransfers: Array<StealthTransfer>- Moving per-transfer fields (
blinded_output_amount,revealed_output_amount,output_memo) into theStealthTransfertypeThis is a clean implementation of the PR's core feature and aligns with the breaking changes notice.
crates/transaction/src/v1/transaction.rs (1)
191-191: LGTM! Weight calculation added for TakeFromBucket.The weight of 1 is appropriate for the
TakeFromBucketinstruction, as it performs a simple workspace bucket operation.crates/p2p/proto/transaction.proto (1)
65-69: LGTM! TakeFromBucket message structure is correct.The message fields are appropriately typed:
input_bucketasWorkspaceOffsetIdfor source bucket referenceamountasAmountfor the quantity to takeoutput_bucketasuint32matching theWorkspaceIdtypecrates/engine/src/transaction/processor.rs (1)
319-336: All edge cases are properly handled—no changes needed.The implementation correctly handles all identified scenarios:
- Insufficient balance: ResourceError::InsufficientBalance is returned with descriptive message showing required vs available amounts
- Invalid bucket reference: RuntimeError::InvalidArgument is returned if bucket_id is missing
- Negative/zero amounts: Template-level assert!(amount.is_positive()) validates amounts
All errors propagate correctly through the ? operator chain in the processor.
applications/tari_walletd/src/handlers/accounts.rs (1)
975-984: LGTM! Clean transformation from request to API parameters.The mapping from
transfersarray toTransferOutputstructs is straightforward and correctly transforms the per-transfer fields. Validation occurs downstream viaparams.validate(network)at line 987, which ensures all constraints are checked before processing.crates/wallet/sdk/src/apis/stealth_transfer/params.rs (4)
30-46: LGTM! Validation correctly enforces constraints.The empty outputs check and blinded output count validation properly address the MAX_LAZY_BP_AGG_FACTORS constraint. Using
is_positive()to count blinded outputs is correct since negative amounts are rejected separately and zero-amount outputs don't contribute to bulletproof aggregation.
48-88: LGTM! Comprehensive per-output validation.The validation logic correctly enforces:
- Non-negative amounts for both blinded and revealed components
- At least one positive amount per output (preventing zero-value outputs)
- Network consistency between destination address and wallet
- Address format validity
93-119: LGTM! Helper methods are clear and correctly implemented.The aggregation methods properly sum amounts across outputs, and the
TransferOutputstruct provides a clean abstraction for per-transfer parameters.
121-136: LGTM! Fallible conversion properly handles errors.The
TryFromimplementation correctly addresses the previous review comment by making the conversion fallible and properly propagating address conversion errors. Using onlyblinded_amount(line 132) is correct, asStealthOutputToCreaterepresents the confidential portion, with revealed amounts handled separately in the transaction flow.crates/wallet/sdk/src/apis/stealth_transfer/api.rs (4)
319-328: LGTM! Per-output destination handling is correctly implemented.The loop properly determines which destination accounts need creation and populates the required substate inputs. Using a
HashSetforaccounts_to_createcorrectly deduplicates when multiple outputs target the same new account. The async calls in the loop are acceptable given the validated output count limit.
593-680: LGTM! Comprehensive destination account determination logic.The helper correctly handles all scenarios:
- Early exit for pure blinded transfers (no account needed)
- Local account optimization to avoid network calls
- Proper differentiation between confirmed and unconfirmed local accounts
- Network queries with appropriate error handling
- Vault detection and input population
The logic properly populates
substate_inputsand returns accurate account creation flags.
497-501: LGTM! Fallible output construction properly propagates errors.The conversion from
TransferOutputtoStealthOutputToCreatecorrectly uses the fallibleTryFromimplementation, ensuring address conversion errors are caught and propagated before transaction generation.
513-520: LGTM! Transfer statement generation correctly combines outputs and change.The chaining of
outputs_to_createwith the optionalchange_outputand filtering for positive amounts ensures only valid outputs are included in the statement.Cargo.toml (1)
1-3: Verify that template version files were updated per the NOTE requirement.Line 1 explicitly documents that template version files must be updated when editing the workspace version. Confirm that the version in
template_built_in/templates/accountandtemplate_built_in/templates/account_nfthas also been bumped to0.14.0to maintain consistency across the workspace.Run the following to verify template versions were updated:
| TakeFromBucket take_from_bucket = 10; | ||
| PublishTemplate publish_template = 11; | ||
| AllocateAddress allocate_address = 12; | ||
| StealthTransfer stealth_transfer = 13; | ||
| PayFee pay_fee = 14; | ||
| } |
There was a problem hiding this comment.
Protobuf field renumbering is a breaking change.
The Instruction oneof fields have been renumbered to accommodate the new TakeFromBucket instruction. This is a breaking protocol change that will cause deserialization failures with older versions.
Impact:
- Existing serialized transactions will fail to deserialize
- Network protocol incompatibility between old and new nodes
- Aligns with PR's breaking change notice: "Requires deletion of the data directory"
If backward compatibility is required, consider:
- Adding
TakeFromBucketat a new field number (e.g., 15) without renumbering existing fields - Implementing a migration path for existing data
If breaking change is acceptable (as indicated by PR):
- Ensure all nodes are upgraded simultaneously
- Verify data directory deletion is properly communicated to users
🤖 Prompt for AI Agents
In crates/p2p/proto/transaction.proto around lines 35–40 the oneof Instruction
fields were renumbered which is a breaking protobuf change; to fix, revert the
existing field numbers for PublishTemplate, AllocateAddress, StealthTransfer,
and PayFee to their original values and add TakeFromBucket using a new unused
field number (e.g., 15) so existing wire-format remains compatible, then
regenerate protobuf bindings and update any references to the new field number;
if a breaking change is intentionally required instead, document the requirement
to upgrade all nodes and to delete or migrate on-disk data and ensure release
notes and upgrade scripts communicate that clearly.
| params.outputs.iter().enumerate().fold(builder, |builder, (i, output)| { | ||
| if !output.revealed_amount.is_positive() { | ||
| return builder; | ||
| } | ||
|
|
||
| let dest_account = derive_account_address_from_public_key(output.address.account_public_key()); | ||
| let need_to_create_account = accounts_to_create.contains(&dest_account); | ||
| let sub_bucket_name = format!("output-sub-bucket-{i}"); | ||
| if need_to_create_account { | ||
| builder | ||
| .take_from_bucket("output_bucket", output.revealed_amount, &sub_bucket_name) | ||
| .create_account_with_bucket( | ||
| *output.address.account_public_key(), | ||
| sub_bucket_name | ||
| ) | ||
| } else { | ||
| builder | ||
| .take_from_bucket("output_bucket", output.revealed_amount, &sub_bucket_name) | ||
| .call_method(dest_account, "deposit", args![Workspace( | ||
| sub_bucket_name | ||
| )]) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Critical issue: Multiple outputs to the same new account will attempt duplicate account creation.
When multiple outputs target the same non-existent destination account with revealed amounts, the current logic will attempt to create the account multiple times within the same transaction (once per output). Line 752 checks a pre-computed accounts_to_create set, which doesn't get updated as accounts are created during transaction building.
Scenario:
- Output 0 → new account A with revealed amount
- Output 1 → same new account A with revealed amount
- Both outputs see
need_to_create_account = true - Both execute
create_account_with_bucket, causing an engine error
Fix: Track accounts created within the transaction generation loop:
.then(|builder| {
+ let mut created_accounts = HashSet::new();
params.outputs.iter().enumerate().fold(builder, |builder, (i, output)| {
if !output.revealed_amount.is_positive() {
return builder;
}
let dest_account = derive_account_address_from_public_key(output.address.account_public_key());
- let need_to_create_account = accounts_to_create.contains(&dest_account);
+ let need_to_create_account = accounts_to_create.contains(&dest_account)
+ && !created_accounts.contains(&dest_account);
let sub_bucket_name = format!("output-sub-bucket-{i}");
if need_to_create_account {
+ created_accounts.insert(dest_account);
builder
.take_from_bucket("output_bucket", output.revealed_amount, &sub_bucket_name)
.create_account_with_bucket(
*output.address.account_public_key(),
sub_bucket_name
)
} else {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/wallet/sdk/src/apis/stealth_transfer/api.rs around lines 746 to 768,
the builder loop uses a precomputed accounts_to_create set so multiple outputs
to the same new account will each try to create the account; change the logic to
track accounts created during transaction construction by introducing a mutable
HashSet (e.g. created_accounts) outside the fold/loop, check
created_accounts.contains(&dest_account) before deciding to create, and when you
perform create_account_with_bucket insert dest_account into created_accounts so
subsequent outputs target that account will call deposit instead of attempting
duplicate creation; ensure the mutable set is accessible inside the closure
(move it or use &mut) and update any types to keep borrow rules satisfied.
Description
feat(engine)!: add TakeFromBucket instruction
feat(wallet)!: support for multiple outputs/stealth transfers in one transaction
Motivation and Context
Allows buckets to be split at the transaction-level
Allows spending to multiple recipients in one transaction
How Has This Been Tested?
New unit tests, manually, existing integration tests
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit
New Features
Refactoring
Chores